Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update timeline items read receipts when the room members are loaded #2194

Conversation

jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Jan 8, 2024

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

When we fetch an item from the timeline cache, we now always update the read receipts with the latest data from the room members list.

Motivation and context

Fixes #2176 .

Tests

  • Open a room (ideally a small-ish one so it doesn't take too long to fetch the room members list).
  • Check the read receipts.

If after a few seconds they change and now contain the right images and user names, it's working.

Tested devices

  • Physical
  • Emulator
  • OS version(s): 14

Checklist

@jmartinesp jmartinesp requested a review from a team as a code owner January 8, 2024 12:09
@jmartinesp jmartinesp requested review from bmarty and removed request for a team January 8, 2024 12:09
Copy link
Contributor

github-actions bot commented Jan 8, 2024

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/FX5i4P

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (da4825a) 70.12% compared to head (765cb97) 70.16%.
Report is 5 commits behind head on develop.

Files Patch % Lines
...es/impl/timeline/factories/TimelineItemsFactory.kt 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2194      +/-   ##
===========================================
+ Coverage    70.12%   70.16%   +0.03%     
===========================================
  Files         1335     1335              
  Lines        32781    32790       +9     
  Branches      6811     6812       +1     
===========================================
+ Hits         22988    23006      +18     
+ Misses        6515     6504      -11     
- Partials      3278     3280       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmartinesp jmartinesp changed the title Update timeline items info when the room members info is loaded Update timeline items read receipts when the room members are loaded Jan 8, 2024
@frebib
Copy link
Contributor

frebib commented Jan 8, 2024

This seems to be a lot more reliable than before 👍. Avatars do all eventually load although it can still take a while (several seconds) in larger rooms as it seems to wait for all avatars to be available before showing them. What would be the performance/UX implications of displaying them one at a time as they're available?
Is there any amount of caching for these images in EXA/rust-sdk? It seems to be slow after first loading the app each time but after that's basically instant.

@jmartinesp
Copy link
Member Author

jmartinesp commented Jan 8, 2024

This seems to be a lot more reliable than before 👍. Avatars do all eventually load although it can still take a while (several seconds) in larger rooms as it seems to wait for all avatars to be available before showing them. What would be the performance/UX implications of displaying them one at a time as they're available? Is there any amount of caching for these images in EXA/rust-sdk? It seems to be slow after first loading the app each time but after that's basically instant.

The issue that makes the images take so long to load is that, although we do have a cache for members in the Rust SDK, if I'm not mistaken (so take it with a grain of salt), the current Room.members() method from the Rust SDK only returns this cached version if it was loaded in the current session / for the current usage of the underlying Room object (I can't remember which one was right).

Technically we could call Room.getMember(userId) for each member at the moment, but it will only return something after this initial member loading has happened. We could call the profile endpoint if I'm not mistaken, but that wouldn't store the results in the Rust cache of room members.

So to improve this in the future I we have 3 options:

  1. Make the Room.members() call return the currently cached members first, then the new ones If my understanding of the current behaviour is right.
  2. Load members in a paginated way so we only load 100/200/500 at a time instead of thousands in large rooms.
  3. Try to lazy load the avatar and display name using the profile endpoint, save them in an alternative cache instead of using the room members one.

Also, loading times on Android are a lot higher than on iOS partly because we have to map those thousand of items to plain Kotlin objects to ensure memory safety given the current UniFFi memory model for Kotlin. This should be fixed in the near future, which should make the app more performant overall, but until then this is what we have.

@frebib
Copy link
Contributor

frebib commented Jan 8, 2024

Thanks for taking the time to explain that!

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but maybe @ganfra can have a look on those changes too.

val updatedItem = if (cacheItem is TimelineItem.Event && roomMembers.isNotEmpty()) {
eventItemFactory.update(
timelineItem = cacheItem,
receivedMatrixTimelineItem = timelineItems[index] as MatrixTimelineItem.Event,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that timelineItems[index] can always be cast to MatrixTimelineItem.Event?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the unique id to identify cached items, so if the cached item is an event I expect the updated item with that id to also be an event. Maybe that's not always the case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the cached value is obviously the same, otherwise there is a big issue :D

@bmarty bmarty requested a review from ganfra January 9, 2024 10:36
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not fix this on the rust sdk instead?
So the Receipt class should have the same logic than ProfileTimelineDetails

@jmartinesp
Copy link
Member Author

Should we not fix this on the rust sdk instead? So the Receipt class should have the same logic than ProfileTimelineDetails

I believe we'd still have this caching issue, even if the SDK also re-emitted the current timeline with the updated read receipts: we'd still be asking the cache for an event with that id, and we'd use the cached one instead of the updated one containing the 'loaded' read receipts.

@jmartinesp
Copy link
Member Author

It seems like we want to go with option 1 for now:

Make the Room.members() call return the currently cached members first, then the new ones If my understanding of the current behaviour is right.

So it's probably better to wait until that is merged in the SDK side to also merge this PR.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay to approve the PR, I think it's fine!

…e-timeline-items-read-receipts-and-sender-info
Copy link

sonarcloud bot commented Jan 23, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 23, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 23, 2024
@jmartinesp jmartinesp merged commit 15e3ecc into develop Jan 24, 2024
18 checks passed
@jmartinesp jmartinesp deleted the fix/jme/2176-update-timeline-items-read-receipts-and-sender-info branch January 24, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read receipt avatars & names sometimes fail to show until the room is reopened
4 participants